Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Create Safe] add overview and CreateSafeInfo widget #1002

Merged
merged 18 commits into from
Nov 3, 2022

Conversation

schmanu
Copy link
Member

@schmanu schmanu commented Oct 28, 2022

What it solves

Resolves #996

How this PR fixes it

Implements the Safe Creation tips as per design
Displays hints for each step + dynamic hints during owner and threshold setup

How to test it

Navigate the Safe Creation flow

Screenshots

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 28, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 19d60fa
Status: ✅  Deploy successful!
Preview URL: https://51e85de0.web-core.pages.dev
Branch Preview URL: https://safe-creation-hints.web-core.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Oct 31, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

- displays safe name and connected wallet during owner setup
- displays hints for each step + dynamic hints during owner and threshold setup
- state is pulled up into the CreateSafe component and update functions passed into the owner step
@DiogoSoaress DiogoSoaress changed the title add overview and CreateSafeInfo widget [Craete Safe] add overview and CreateSafeInfo widget Nov 1, 2022
@DiogoSoaress DiogoSoaress changed the title [Craete Safe] add overview and CreateSafeInfo widget [Create Safe] add overview and CreateSafeInfo widget Nov 1, 2022
@DiogoSoaress DiogoSoaress marked this pull request as ready for review November 1, 2022 11:41
@usame-algan usame-algan requested a review from iamacook November 2, 2022 08:18
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very sleek!

src/components/create-safe/InfoWidget/index.tsx Outdated Show resolved Hide resolved
src/components/create-safe/InfoWidget/index.tsx Outdated Show resolved Hide resolved
src/components/create-safe/InfoWidget/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/CardStepper/useCardStepper.ts Outdated Show resolved Hide resolved
src/components/new-safe/CreateSafe/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/CreateSafe/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/CreateSafe/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/steps/Step1/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/steps/Step1/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/steps/Step2/index.tsx Outdated Show resolved Hide resolved
@DiogoSoaress DiogoSoaress requested a review from iamacook November 2, 2022 14:09
<>
Not sure how many owners and confirmations you need for your Safe?{' '}
<Link
sx={{ textDecoration: 'underline' }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we override the global link styles? cc @liliiaorlenko

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a mockup for the link. Mostly because it was only suggested in this PR. I suggest it should not block this one and rather be addressed after a final design review on the Create flow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that our theme includes link styling and underlining it here does not conform with it. We can revisit this though.

src/components/new-safe/CreateSafe/index.tsx Outdated Show resolved Hide resolved
src/components/new-safe/steps/Step2/index.tsx Outdated Show resolved Hide resolved
<Box p={2} sx={{ backgroundColor: 'background.main', borderRadius: '8px' }}>
<Typography variant="subtitle1" fontWeight={700} display="inline-flex" alignItems="center" gap={1}>
Safe Mobile owner key (optional){' '}
<Tooltip title="TODO: Add tooltip" arrow placement="top">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a tooltip title yet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I think it can also figure out in a different PR. wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found them. I'm gonna add it

Screenshot 2022-11-02 at 18 12 13

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm waiting for the urls to include in the tooltip link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest we include the links in the standard text and include a summary in the tooltips. It's odd to "hide" links. cc @liliiaorlenko

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we go with a short tooltip copy here instead of links? Also let's capitalize on the tips that we are displaying on the side.

Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I suggest we include the missing tooltips in another PR as they are not related to this task.

@DiogoSoaress DiogoSoaress merged commit 6b05e27 into creation Nov 3, 2022
@DiogoSoaress DiogoSoaress deleted the safe-creation-hints branch November 3, 2022 12:25
usame-algan added a commit that referenced this pull request Nov 24, 2022
* feat: new creation design + step 1

* fix: widget width

* refactor: combine components

* fix: rename folder + remove CSS module

* fix: missing padding

* fix: Reuse NetworkSelector component

* [Safe Creation] Owner step (#989)

* feat: basic step 2 implementation

Co-authored-by: iamacook <aaron@safe.global>

* feat: creation info widget (#960)

* feat: creation info widget

* fix: Remove demo page

Co-authored-by: Usame Algan <usame@safe.global>

* Safe creation stepper (#992)

A new CardStepper component which controls the steps for new safe creation which can
- go back and forth
- jump to a specific step
- set an initial step
- set initial values
- renders a progress bar
- step data is stored in an object with a generic type
  - the update functions (onBack, onSubmit) use Partials of that type such that each step does not need to submit the full data

change /demo route to /create-safe

Co-authored-by: Usame Algan <usame@safe.global>

* Review step redesign (#993)

* feat: new create safe review step

* calculate total gas fee

* fill the form values

* refactor review rows

* fix: implement Figma designs

* fix: background-color on network fee

* avoid infinite rerendering

* integrate review step in new Stepper

* style: visual tweaks

* fix: Create safe layout adjustments (#997)

* fix: Create safe layout adjustments

* fix: overview widget margin top

* fix: Move overview widget to top on mobile

* feat: AB testing system (#1013)

* feat: AB testing system

* fix: remove reset function + test `AbTest` value

* Refactor: sync useLocalStorage across components and tabs (#977)

* Refactor: external store in useLocalStorage

* Comment

* Sync separately

* Use the storage event for syncing

* Rm comment

* Restore prev behavior wrt undefined

* Simplify the undefined check

* Allow undefined and rm initialValue

* Don't set undefined

* Local storage to return null when not found

* Initial value

* Rm initial value again

* usePendingCreation

* Rm redundant code

* Refactor: external store in useLocalStorage (#1014)

Refactor: external store in useLocalStorage

* feat: create `useABTesting` hook for setting event

* fix: reorganise structure

Co-authored-by: katspaugh <381895+katspaugh@users.noreply.github.com>

* feat: add AB testing to Safe creation (#1017)

* [Create Safe] feat: Connect wallet step (#1006)

* fix: Add connect wallet step to safe creation

* fix: Remove unused method

* fix: Reuse components, adjust modal zIndex

* fix: Only run onConnect callback if there was no error when connecting a wallet

* fix: Set wallet owner in form on connect

* fix: Check for connected wallet

* style: Use onboard zindex variable

* [Create Safe] add overview and CreateSafeInfo widget (#1002)

* add overview and CreateSafeInfo widget

- displays safe name and connected wallet during owner setup
- displays hints for each step + dynamic hints during owner and threshold setup
- state is pulled up into the CreateSafe component and update functions passed into the owner step

* step static tips are collapsed by default

* navigate tips logic

* reverse expanded logic

* restyle InfoWidget

* remove the add mobile owner row

* remove flickering when opening the accordion

* fix keys

* add hover style to the Accordion expand icon

* Accordion summary bold when expanded

* style: PR comments

* fix empty form data name

* simplify map key

* link to Safe setup article

* change allOwners logic

* mv sx to css modules

* add current chain in Overview widget

Co-authored-by: Diogo Soares <diogo.lsoares@gmail.com>

* [Create Safe] feat: Status view redesign (#1018)

* feat: Implement create safe status redesign

* style: Add animated loading spinner

* fix: Simplify conditions

* add: animate into css safe logo

* fix: use numeric enum for simpler conditions, adjust animated logo sizes

* fix: Display dialog after safe creation

* fix: Feedback

* fix: Better name for query param

Co-authored-by: schmanu <manu@safe.global>

* [Safe creation] Creation links tooltips (#1061)

* add link to tip

* add missing tooltips

* update copy

* use SvgIcon to inherit viewBox

* fix: inherit `fill`

Co-authored-by: iamacook <aaron@safe.global>

* fix: textual inconsistencies + add hint (#1065)

* [Safe Creation] Layout adjustments (#1063)

* style: Safe Creation layout

* fix: Align threshold text

* [Create Safe] Handle wallet connection (#1087)

* fix: Handle wallet connection when creating safe

* fix: Only watch threshold

* refactor: Rename useSetCreationStep to useSyncSafeCreationStep

* fix: Use formState.isValid

* fix: Add tests for useSyncSafeCreationStep

* fix: Show error notifications in safe creation (#1135)

* fix: Show error notifications in safe creation

* fix: Add closeByGroupKey action to notification slice

* fix: Safe creation issues (#1180)

* fix: Navigate back to the welcome page on cancel

* fix: Reuse NameInput for new form

* fix: Use currentColor for icons

* fix: check for pending safe creation in first step (#1190)

* fix: reduce threshold when removing owner (#1181)

* fix: reduce threshold when removing owner

* fix: use enum for all field names

* fix: reduce code

* Safe creation text updates (#1198)

* fix: Update text and layout for step 0

* fix: Update text for step 2 and 3, show copy and explorer icons for addresses

* fix: Update text for status messages

* fix: Update hint texts

* fix: Update creation modal text, add dynamic values

* fix: QR reader crashes app (#1200)

* fix: Safe Creation color issues (#1204)

* fix: Safe Creation color issues

* chore: Add TODO

* fix: Remove static colors and use palette directly

* fix: Add overview widget to step 1 in safe creation (#1210)

* fix: Add missing safe creation events (#1212)

* fix: Add missing safe creation events

* fix: Add new events for hints

* fix: Watch safe creation tx even without a connected wallet (#1215)

* fix: Watch safe creation tx even without a connected wallet

* fix it in the old safe creation flow too

* fix: failing test

* fix: Add additional test

* fix: Only watch safe creation tx once (#1223)

* fix: Only watch safe creation tx once

* fix: Detect if tx reverted when ethers throws error

* refactor: Remove unused code that was duplicated (#1230)

* refactor: Remove unused code that was duplicated

* fix: Revert ab test removal

* refactor: Remove unused StepCard component

* fix: Update links to safe.global

* fix: Set link to safe creation for ab testing

* fix: Hide sidebar on safe creation

* fix: Revert ab test change

* fix: Center QR code reload icon

* fix: Safe cration text changes (#1239)

* fix: Safe cration text changes

* fix: Adjust safe indexed message

* fix: Adjust safe indexed message

Co-authored-by: iamacook <aaron@safe.global>
Co-authored-by: Manuel Gellfart <manu@safe.global>
Co-authored-by: Diogo Soares <32431609+DiogoSoaress@users.noreply.github.com>
Co-authored-by: katspaugh <381895+katspaugh@users.noreply.github.com>
Co-authored-by: Diogo Soares <diogo.lsoares@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants